-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
105 add weights of evidence #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! There are mainly some style related comments at this points. I added some comments for specific files and lines, but here are some general comments:
- Remember to run the linter. I think a lot of minor problems could be automatically fixed with that, so I suggest you do it before doing manually any modifications. Nikolas recently merged a configuration file for a tool called pre-commit in the master branch, which I suggest you utilize. You would first need to pull the latest updates from the master branch to your wofe branch so that you get the configuration file. You can see the contributing.md for instructions on how to run pre-commit. It is fine to use the old way (
poetry run invoke lint
), but the pre-commit is very useful and maybe a bit clearer in my opinion. It will also automatically modify your files where it can. - Please use longer, more meaningful parameter names, as now it was very difficult for me to understand some parts of the code. Of course we want to avoid super long variable names, say 30+ characters, but clear naming is important for PR reviewing and maintainibility.
- Remember that there is max line length defined by flake8 (120 characters), but this is also checked by the linter
- When I was reading this PR, I came to think that we might want to be more flexible with the public-private -functions style. Now there are many helper/utility functions that are used to generate the desired output, but that are not meant for a user directly (as far as I know). So, I propose that you get rid of the public/outer functions for everything else except for the weights_calculations, and that you rename this file to just weights_of_evidence.py, and that you create a second file called weights_of_evidence_functions.py or weights_of_evidence_utils.py where you will put all the other (private) functions. And, maybe you could also fit the contents of basic_calculations and calculate_weights inside the private main function for weights_of_evidence. Please correct me if I'm wrong and some of these functions are meant to be exposed for the user, but if not, I think we can pack the helper functions more concisely and clearer in just one file. Then you could also get rid of the weights_of_evidence folder.
- Lastly, I did not really check the logic and tests that much yet, but I will pay more mind to this in the second review round
.vscode/settings.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can keep this out of the remote repository, just locally. So this could be added to .gitignore and removed from the PR.
eis_toolkit/prediction/weights_of_evidence/basic_calculations.py
Outdated
Show resolved
Hide resolved
eis_toolkit/prediction/weights_of_evidence/basic_calculations.py
Outdated
Show resolved
Hide resolved
eis_toolkit/prediction/weights_of_evidence/weights_calculations.py
Outdated
Show resolved
Hide resolved
eis_toolkit/prediction/weights_of_evidence/weights_calculations.py
Outdated
Show resolved
Hide resolved
eis_toolkit/prediction/weights_of_evidence/weights_generalizations.py
Outdated
Show resolved
Hide resolved
…me other minor changes
Thanks for the review @nmaarnio. I have implemented the most relevant changes. Haven't delved much into the coding style and structuring related issues. But if you notice anything disturbingly out of order, let me know. Other things seem functional at this point. |
…put (WIP), fix discrepancies in B=0 handling (WIP)
…nal nodata parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests should be added, even simple ones. The notebook uses data from tests/data/local
which means the data is not in the GitHub repository available for testing. The notebook should either provide a means of downloading the local data, use data that is in the repository or maybe be removed?
…iles, adjusted documentation
Thanks for the review @nialov . I added minimal test file and made changes according to your review, but kept the dosctring same for clarity. |
Basic implementation of weights of evidence and posterior probability calculations is now finished. There will be improvements and modification in the future, @chudasama-bijal or someone else will make an issue listing the desired modifications. |
Added the functions related to implementation of weights-of-evidence method for predictive mapping.